feat(ecs): Add SystemControls to ContainerDefinition #16970
Conversation
|
|
|
Hi @madeline-k , mind taking a look at this? :) |
There was a problem hiding this comment.
Hey @ayush987goyal, thanks for opening this PR!
- Even though the feature is small, please add a unit test, testing the
systemControlsoptions. - If there is not an existing README section that makes sense to add this feature to, please create a new one! The README addition can be as short as just a one-two sentence description of what the feature is, and a code example using it.
|
oh, just saw that you already extended an existing unit test. So, this is probably okay on testing. But if you think it makes sense to add an additional unit test testing the minimal ContainerDefinition using this new option, that could be useful. |
mergify
bot
dismissed
madeline-k’s stale review
Pull request has been modified.
peterwoodworth
changed the title
github-actions
bot
added
the
@aws-cdk/aws-ecs
|
@madeline-k Just checking in to see if you got a chance to take a look at this change? |
| declare const taskDefinition: ecs.TaskDefinition; | ||
|
|
||
| taskDefinition.addContainer('container', { |
There was a problem hiding this comment.
Is this a working example? Doesn't this need to actually instantiate a TaskDefinition first?
There was a problem hiding this comment.
Yes this is not a working example since the section is concerned about the container definition of an existing taskDefinition. The same is being followed across the README.
mergify
bot
dismissed
madeline-k’s stale review
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
feat(ecs): Add SystemControls to ContainerDefinition closes aws#16025 Note: 1. Couldn't find the good place in the current readme for this feature to be added. So the PR linter would fail. Let me know if there is indeed a correct place to put this in any readme. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
feat(ecs): Add SystemControls to ContainerDefinition
closes #16025
Note:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license